-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: capture error stacks across async calls #1088
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You showed me a test case for this earlier - would it be possible to add a single System Test for one of the entry points?
I also wonder if Errror().stack
generates some overhead since the documentation says that "stack" is lazily generated. If you have time, can you benchmark a single call?
dev/src/bulk-writer.ts
Outdated
return this.writeBatch.bulkCommit(); | ||
|
||
// Capture the error stack to preserve stack tracing across async calls. | ||
const stack = Error().stack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Move bang here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
dev/src/index.ts
Outdated
@@ -908,9 +908,15 @@ export class Firestore { | |||
documentRefsOrReadOptions | |||
); | |||
const tag = requestTag(); | |||
|
|||
// Capture the error stack to preserve stack tracing across async calls. | |||
const stack = Error().stack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Move bang here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
dev/src/pool.ts
Outdated
@@ -17,7 +17,7 @@ | |||
import * as assert from 'assert'; | |||
|
|||
import {logger} from './logger'; | |||
import {Deferred} from './util'; | |||
import {Deferred, wrapError} from './util'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed.
dev/src/reference.ts
Outdated
@@ -1808,12 +1808,15 @@ export class Query<T = DocumentData> { | |||
_get(transactionId?: Uint8Array): Promise<QuerySnapshot<T>> { | |||
const docs: Array<QueryDocumentSnapshot<T>> = []; | |||
|
|||
// Capture the error stack to preserve stack tracing across async calls. | |||
const stack = Error().stack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Move bang here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
dev/src/write-batch.ts
Outdated
@@ -532,7 +532,13 @@ export class WriteBatch { | |||
* }); | |||
*/ | |||
commit(): Promise<WriteResult[]> { | |||
return this.commit_(); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Remove empty line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
dev/src/write-batch.ts
Outdated
return this.commit_(); | ||
|
||
// Capture the error stack to preserve stack tracing across async calls. | ||
const stack = Error().stack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Move bang here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some benchmarking. Across 2500 calls to ref.get()
the average RTT is 63.4ms for stack tracing vs. 60.1ms for no stack tracing. The spread was pretty wide though, with times in the +/- 10ms range. Calls to ref.set()
were similar.
dev/src/bulk-writer.ts
Outdated
return this.writeBatch.bulkCommit(); | ||
|
||
// Capture the error stack to preserve stack tracing across async calls. | ||
const stack = Error().stack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
dev/src/index.ts
Outdated
@@ -908,9 +908,15 @@ export class Firestore { | |||
documentRefsOrReadOptions | |||
); | |||
const tag = requestTag(); | |||
|
|||
// Capture the error stack to preserve stack tracing across async calls. | |||
const stack = Error().stack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
dev/src/pool.ts
Outdated
@@ -17,7 +17,7 @@ | |||
import * as assert from 'assert'; | |||
|
|||
import {logger} from './logger'; | |||
import {Deferred} from './util'; | |||
import {Deferred, wrapError} from './util'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed.
dev/src/reference.ts
Outdated
@@ -1808,12 +1808,15 @@ export class Query<T = DocumentData> { | |||
_get(transactionId?: Uint8Array): Promise<QuerySnapshot<T>> { | |||
const docs: Array<QueryDocumentSnapshot<T>> = []; | |||
|
|||
// Capture the error stack to preserve stack tracing across async calls. | |||
const stack = Error().stack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
dev/src/write-batch.ts
Outdated
@@ -532,7 +532,13 @@ export class WriteBatch { | |||
* }); | |||
*/ | |||
commit(): Promise<WriteResult[]> { | |||
return this.commit_(); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
dev/src/write-batch.ts
Outdated
return this.commit_(); | ||
|
||
// Capture the error stack to preserve stack tracing across async calls. | ||
const stack = Error().stack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
d99b124
to
541449a
Compare
dev/system-test/firestore.ts
Outdated
const ref = randomCol.doc('__doc__'); | ||
const batch = firestore.batch(); | ||
batch.set(ref, {foo: 'a'}); | ||
return batch.commit().then(() => Promise.reject('commit() should have failed') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am glad this is causing a lint failure :) Doesn't look pretty like this. The PR is good to go after yarn gts fix
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. Also I changed wrapError()
to use GoogleError
in order to preserve error codes when re-throwing.
…re into bc/error-stack
Codecov Report
@@ Coverage Diff @@
## master #1088 +/- ##
==========================================
- Coverage 97.62% 97.48% -0.15%
==========================================
Files 27 27
Lines 16924 16960 +36
Branches 1347 1267 -80
==========================================
+ Hits 16522 16533 +11
- Misses 399 423 +24
- Partials 3 4 +1
Continue to review full report at Codecov.
|
dev/src/util.ts
Outdated
export function wrapError(err: GoogleError, stack: string): Error { | ||
const wrappedError = new GoogleError(err.message); | ||
wrappedError.code = err.code; | ||
wrappedError.stack += '\nCaused by: ' + stack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we modify the stack in 'err' directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #1080.
Tested for get/set operations on references, queries, batches, and bulkCommit.